Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev/listener multiplexer #1542

Merged
merged 49 commits into from
Aug 28, 2021
Merged

Conversation

bakape
Copy link
Contributor

@bakape bakape commented Sep 3, 2020

Description

Reduces the overhead of creating and dropping event listeners on each patch by registering them inside a global multiplexer. Also provides opt-in callback flags to additionally increase performance at the user's discretion.

Features:

  • Multiplexer core functionality
  • Callback API extensions
  • Listener module porting
  • Passive listeners
  • Deferred listeners
  • Event bubbling
    • Synchronous listeners
    • Passive listeners
    • Deferred listeners
    • Event bubbling
    • Input listeners
    • Change listeners

The core functionality is done, so making this ahead of completion to potentially speed up the review.

Fixes #943

Breaking change

Handling bubbled events by default is expensive. This is the current behaviour in yew. With this PR events do not bubble by default and instead users have to either make a small change to their code or opt into a global deoptimization for that specific event kind.

Example:

<a onclick=self.link.callback(|| ())>
    <span></span>
</a>

becomes

let cb = self.link.callback(|| ());
<a onclick=cb.clone()>
    <span onclick=cb></span>
</a>

Or, in case the user really wants event bubbling:

<a onclick=self.link.callback_with_flags(HANDLE_BUBBLING, || ())>
    <span>
	<span></span>
    	<span></span>	
    </span>
    <span></span>
    <span></span>
    <span></span>
    <span></span>
</a>

This causes all click events globally to perform upward DOM tree traversal to find any other listeners to call.

Checklist:

Copy link
Member

@siku2 siku2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take a closer look at the code itself, here are just a few things I noticed.

ci/run_tests.sh Outdated Show resolved Hide resolved
yew/src/callback.rs Outdated Show resolved Hide resolved
yew/src/callback.rs Outdated Show resolved Hide resolved
yew/src/html/listener/listener_stdweb.rs Outdated Show resolved Hide resolved
yew/src/html/listener/listener_web_sys.rs Outdated Show resolved Hide resolved
yew/src/html/listener/listener_web_sys.rs Outdated Show resolved Hide resolved
yew/src/html/listener/listener_web_sys.rs Outdated Show resolved Hide resolved
yew/src/html/listener/listener_web_sys.rs Outdated Show resolved Hide resolved
yew/src/callback.rs Outdated Show resolved Hide resolved
yew/Cargo.toml Outdated Show resolved Hide resolved
@jstarry
Copy link
Member

jstarry commented Sep 8, 2020

Handling bubbled events by default is expensive. This is the current behaviour in yew. With this PR events do not bubble by default and instead users have to either make a small change to their code or opt into a global deoptimization for that specific event kind.

I feel that enabling event bubbling is a better dev experience. I would prefer it to be the default, but devs can choose to optimize for performance and disable if it needed.

I just ran perf benchmarks. The partial update improvements are great and expected! I'm curious what made row creation slow down though, any ideas?

Screen Shot 2020-09-08 at 11 47 09 PM

@bakape
Copy link
Contributor Author

bakape commented Sep 8, 2020

I feel that enabling event bubbling is a better dev experience. I would prefer it to be the default, but devs can choose to optimize for performance and disable if it needed.

So a feature to not add HANDLE_BUBBLED to the callback flags by default?

I just ran perf benchmarks. The partial update improvements are great and expected! I'm curious what made row creation slow down though, any idea?

I'd bet it's the listener equality check. I'm not too familiar with the VTag lifecycle yet. Does a diff on the exact same VTag (same view() call) ever happen? As far as I understand, that is the only scenario, when listeners can be equal. I have a feeling it's actually faster to never check listener equality and always replace them.

@bakape bakape marked this pull request as ready for review September 11, 2020 20:06
@bakape
Copy link
Contributor Author

bakape commented Sep 11, 2020

@jstarry Listener registration optimized.

@bakape
Copy link
Contributor Author

bakape commented Aug 15, 2021

@siku2 @jstarry Resolved all mentioned issues, reduced the amount of features, API changes and cognitive load due to configuration variations. Benchmark results still look good.

@mc1098 I see this conflicts with some of your PRs, so you might want to take a look as well.

@mc1098
Copy link
Contributor

mc1098 commented Aug 15, 2021

The conflicts are fine :)

siku2
siku2 previously approved these changes Aug 16, 2021
packages/yew/src/html/listener/mod.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed siku2’s stale review August 21, 2021 17:32

Pull request has been modified.

@bakape
Copy link
Contributor Author

bakape commented Aug 28, 2021

@siku2 Master merged and all issues/comments addressed.

@bakape
Copy link
Contributor Author

bakape commented Aug 28, 2021

Right, Components v2. I'll push a fix for benchmarks a bit later. This can be merged as is.

@siku2 siku2 merged commit 68d2fdb into yewstack:master Aug 28, 2021
@mc1098 mc1098 mentioned this pull request Aug 28, 2021
3 tasks
mc1098 pushed a commit to mc1098/yew that referenced this pull request Aug 31, 2021
PR yewstack#1542 didn't merge in the changes from yewstack#1989 correctly, this is fixed
in this commit.
siku2 pushed a commit that referenced this pull request Aug 31, 2021
PR #1542 didn't merge in the changes from #1989 correctly, this is fixed
in this commit.
@mc1098 mc1098 mentioned this pull request Sep 11, 2021
3 tasks
@mc1098 mc1098 mentioned this pull request Oct 12, 2021
3 tasks
@voidpumpkin voidpumpkin added the A-yew Area: The main yew crate label Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[virtual_dom/vtag.rs] Compare references of handler to do listeners update better
5 participants